Fix unchecked integer overflow in encoder UV plane allocation by jortles · Pull Request #3233 · AOMediaCodec/libavif

jortles · 2026-05-29T18:48:14Z

Summary

Add pre-multiplication overflow checks to codec_avm.c and codec_svt.c for UV plane size computations, consistent with the existing safe pattern in avifImageAllocatePlanes() (avif.c:435-441)
Add overflow check in avifImageCopyProperties() before numProperties allocation, consistent with avifImagePushProperty() (avif.c:387)

Details

Three sites compute allocation sizes via unchecked integer multiplication, unlike their parallel functions which validate before multiplying:

codec_avm.c:919 — channelSize * monoUVWidth (uint32_t) and monoUVHeight * monoUVRowBytes (size_t) used for avifAlloc() without overflow checks. The parallel code in avifImageAllocatePlanes() checks width > UINT32_MAX / channelSize and height > PTRDIFF_MAX / fullRowBytes before the same operations.

codec_svt.c:286 — uvWidth * bytesPerPixel (uint32_t) and uvHeight * uvRowBytes (size_t) computed without pre-multiplication validation. The existing post-hoc check (uvSize > UINT32_MAX / 2) cannot detect a wrap that already occurred.

avif.c:237 — numProperties * sizeof(...) passed to avifAlloc() without an overflow guard. avifImagePushProperty() in the same file already checks numProperties < SIZE_MAX / sizeof(avifImageItemProperty) before an identical allocation.

Test plan

Builds cleanly with cmake --build . --target avif_obj (zero warnings)
Existing test suite passes (CI)

Add pre-multiplication overflow checks to codec_avm.c and codec_svt.c for UV plane size computations, consistent with the existing safe pattern in avifImageAllocatePlanes() (avif.c:435-441). Also add an overflow check in avifImageCopyProperties() before the numProperties allocation, consistent with avifImagePushProperty() (avif.c:387). Without these checks, crafted image dimensions can silently wrap the uint32_t multiplication, leading to undersized allocations. Signed-off-by: Anthony Hurtado <amhurtado@pm.me>

y-guyon

Thank you for your interest in libavif.

y-guyon · 2026-06-01T07:57:29Z

            // Allocate the U plane if necessary.
            if (!avmImageAllocated) {
                uint32_t channelSize = avifImageUsesU16(image) ? 2 : 1;
+                if (monoUVWidth > UINT32_MAX / channelSize) {


monoUVWidth is always below UINT32_MAX / channelSize because channelSize is at most 2 and monoUVWidth = (image->width + 1) >> 1.

However I wonder if there should be a check for the image->width + 1 part. Maybe avm_codec_enc_init() fails earlier in case of a huge dimension, but that could change in the future.

Good catch — you're right that monoUVWidth can never exceed UINT32_MAX / channelSize given the current math. I've removed that unreachable check and added a guard for the image->width + 1 overflow instead, which is the actual risk. Same change applied to codec_svt.c.

y-guyon · 2026-06-01T08:05:41Z

+                    return AVIF_RESULT_INVALID_ARGUMENT;
+                }
                uint32_t monoUVRowBytes = channelSize * monoUVWidth;
+                if (monoUVHeight > PTRDIFF_MAX / monoUVRowBytes) {


I see these checks were inspired from:

libavif/src/avif.c

Lines 433 to 440 in d8b4e04

const uint32_t channelSize = avifImageUsesU16(image) ? 2 : 1;

if (image->width > UINT32_MAX / channelSize) {

return AVIF_RESULT_INVALID_ARGUMENT;

}

const uint32_t fullRowBytes = channelSize * image->width;

if (image->height > PTRDIFF_MAX / fullRowBytes) {

return AVIF_RESULT_INVALID_ARGUMENT;

}

But PTRDIFF_MAX makes little sense in both places in my opinion. It should be SIZE_MAX right?

Agreed — SIZE_MAX is the correct bound here. Changed in both codec_avm.c and codec_svt.c.

- Add UINT32_MAX dimension guard before (width+1)>>1 to prevent the +1 overflow, as suggested by reviewer - Remove unreachable monoUVWidth > UINT32_MAX/channelSize check (provably safe given the dimension guard) - Change PTRDIFF_MAX to SIZE_MAX for size_t allocation bound Signed-off-by: Anthony Hurtado <amhurtado@pm.me>

y-guyon · 2026-06-01T15:31:47Z

            // monochrome. Manually set UV planes to 0.5.

            // avmImage is always 420 when we're monochrome
+            if (image->width == UINT32_MAX || image->height == UINT32_MAX) {


Maybe image->width should be cast to size_t instead of compared against UINT32_MAX? It does not matter much for such huge dimensions I guess.

Yes, we should allow UINT32_MAX.

As Yannis suggested, we can cast image->width to a larger type such as uint64_t. (size_t is 32 bits in 32-bit systems.) We use this approach elsewhere.

Another approach is to replace (image->width + 1) >> 1 with image->width / 2 + image->width % 2, or (since image->width >= 1) 1 + (image->width - 1) / 2, or some other similar trick.

I just realized that by the time we reach here, image->width and image->height have been validated by avifValidateGrid() to be valid AV1 frame width and height. So we know image->width <= 65536 and image->height <= 65536. Therefore we should simply delete this if statement.

Are there the same frame dimension limits in AV2?

In general it could be AVIF_ASSERT_OR_RETURN() instead of checks, but leaving the code as is is fine with me too.

Yannis: Thank you for checking my analysis. This was a flaw in my analysis. I just checked the AV2 specification (version 1.0.0). AV2 has the same frame dimension limits as AV1. (We can verify this by searching for "max_frame_width_minus_1" and "max_frame_height_minus_1" in the AV2 spec.) I just wrote PR #3238 to note this fact.

wantehchang

Anthony: After merging PR #3210, and after analyzing the changes to codec_avm.c and codec_svt.c carefully, I concluded that we don't need any of the changes in this PR. So I am going to close this PR.

Please check if my analysis is correct. Thank you!

wantehchang · 2026-06-01T21:21:57Z

    dstImage->numProperties = 0;

    if (srcImage->numProperties != 0) {
+        AVIF_CHECKERR(srcImage->numProperties < SIZE_MAX / sizeof(srcImage->properties[0]), AVIF_RESULT_INVALID_ARGUMENT);


Please revert the change to this file. This has been addressed by PR #3210.

wantehchang · 2026-06-01T21:35:27Z

            // monochrome. Manually set UV planes to 0.5.

            // avmImage is always 420 when we're monochrome
+            if (image->width == UINT32_MAX || image->height == UINT32_MAX) {


I just realized that by the time we reach here, image->width and image->height have been validated by avifValidateGrid() to be valid AV1 frame width and height. So we know image->width <= 65536 and image->height <= 65536. Therefore we should simply delete this if statement.

wantehchang · 2026-06-01T21:40:19Z

                uint32_t monoUVRowBytes = channelSize * monoUVWidth;
+                if (monoUVHeight > SIZE_MAX / monoUVRowBytes) {
+                    return AVIF_RESULT_INVALID_ARGUMENT;
+                }


We should also delete this if statement because:

monoUVWidth <= 32768 monoUVHeight <= 32768 channelSize <= 2 monoUVHeight * monoUVRowBytes <= 32768 * 2 * 32768 < UINT32_MAX <= SIZE_MAX

wantehchang · 2026-06-01T21:45:04Z

        const uint32_t uvRowBytes = uvWidth * bytesPerPixel;
+        if (uvHeight > SIZE_MAX / uvRowBytes) {
+            goto cleanup;
+        }


y_shift is equal to 1 for 4:2:0. So my analysis in codec_avm.c also applies here. We should delete these two if statements.

Copilot AI review requested due to automatic review settings May 29, 2026 18:48

y-guyon reviewed Jun 1, 2026

View reviewed changes

wantehchang reviewed Jun 1, 2026

View reviewed changes

wantehchang closed this Jun 1, 2026

	const uint32_t channelSize = avifImageUsesU16(image) ? 2 : 1;
	if (image->width > UINT32_MAX / channelSize) {
	return AVIF_RESULT_INVALID_ARGUMENT;
	}
	const uint32_t fullRowBytes = channelSize * image->width;
	if (image->height > PTRDIFF_MAX / fullRowBytes) {
	return AVIF_RESULT_INVALID_ARGUMENT;
	}

Conversation

jortles commented May 29, 2026

Summary

Details

Test plan

Uh oh!

y-guyon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wantehchang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants